Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] 32/64-bit float consistency with BernoulliRBM #16352

Merged
merged 14 commits into from Jun 23, 2020

Conversation

Henley13
Copy link
Contributor

Reference Issues/PRs

Works on #11000 for BernoulliRBM.

What does this implement/fix? Explain your changes.

Prevent the transformer from converting float32 to float64.

Any other comments?

Maybe we should wait to merge a generic test for dtype consistency (see #16290) before merging this one.

@Henley13 Henley13 changed the title [MRG] Bernoulli preserving dtypes [MRG] 32/64-bit float consistency with BernoulliRBM Jan 31, 2020

# dtype_in and dtype_out consistent
assert Xt.dtype == dtype_out, ('transform dtype: {} - original dtype: {}'
.format(Xt.dtype, X.dtype))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While #16290 is not merged, we also should add a check that the results of fit_transform are close enough with float32 and float64 input.

Copy link
Contributor

@ksslng ksslng Feb 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also should check that the attributes of the estimator are close on float32 and float64 inputs, which I think is better to be done in the individual tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added a dedicated test for that.

Copy link
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this more susceptible to loss of precision in intermediate matrix multiplications?

@rth
Copy link
Member

rth commented Feb 3, 2020

Is this more susceptible to loss of precision in intermediate matrix multiplications?

I guess this question could apply to any neural net architecture, and yet all DL libraries use float32 and sometimes even floa16 lately. Maybe they just don't enforce that outputs in f64/f32 are identical? Not sure. If so, I wonder whether we should. Possibly checking that the convergence critera was reached no matter what it was is enough (at least for some type of algorithms).

@rth rth added the Needs work label Feb 5, 2020
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. Just a small comment. Also please add a what's new entry

Comment on lines 224 to 232
assert_almost_equal(Xt_64, Xt_32, 6)
assert_almost_equal(rbm_64.intercept_hidden_,
rbm_32.intercept_hidden_,
6)
assert_almost_equal(rbm_64.intercept_visible_,
rbm_32.intercept_visible_,
6)
assert_almost_equal(rbm_64.components_, rbm_32.components_, 6)
assert_almost_equal(rbm_64.h_samples_, rbm_32.h_samples_, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use assert_allclose instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -146,8 +146,10 @@ def _mean_hiddens(self, v):
h : ndarray of shape (n_samples, n_components)
Corresponding mean field values for the hidden layer.
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

p = safe_sparse_dot(v, self.components_.T)
p += self.intercept_hidden_

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@@ -235,6 +235,10 @@ Changelog
:class:`neural_network.MLPClassifier` by clipping the probabilities.
:pr:`16117` by `Thomas Fan`_.

- |Enhancement| Prevent the transformer from converting float32 to float64 in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "Avoid converting float32 input to float64 in ..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better ^^

@rth rth removed the Needs work label Jun 23, 2020
@rth rth merged commit 3583149 into scikit-learn:master Jun 23, 2020
@rth
Copy link
Member

rth commented Jun 23, 2020

Thanks @Henley13 !

rubywerman pushed a commit to MLH-Fellowship/scikit-learn that referenced this pull request Jun 24, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants